Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Don't reset retirement_requester after end of retirement #18325

Merged
merged 1 commit into from
Jan 3, 2019
Merged

Don't reset retirement_requester after end of retirement #18325

merged 1 commit into from
Jan 3, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 3, 2019

Because we have a column name and a method name that are equivalent, finish_retirement used to reset the retirement_requester to nil. Changing the method to call a different method that only sets the field name prevents this from being reset to nil at the end.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1638502

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 3, 2019

@miq-bot add_label bug, blocker
@miq-bot assign @gmcculloug
@miq-bot add_reviewer @tinaafitz
@miq-bot add_reviewer @mkanoor

@d-m-u d-m-u changed the title Not reset retirement_requester after end of retirement Don't reset retirement_requester after end of retirement Jan 3, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 3, 2019

@miq-bot add_label hammer/yes

@d-m-u d-m-u force-pushed the fixing_retirement_requester_reset branch from f787282 to f4eb967 Compare January 3, 2019 17:59
gmcculloug
gmcculloug approved these changes Jan 3, 2019
View reviewed changes
Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

The one thing I would suggest, in a followup PR, is to update the #mark_retired tests to use a Timecop.freeze block around the retires_on date check instead of testing against a large time range.

For example, the check
expect(@service.retires_on).to be_between(Time.zone.now - 1.hour, Time.zone.now + 1.second)

Should really be an exact match if time is frozen.

This currently happens in all three specs being updated here.

@tinaafitz
Copy link
Member

@d-m-u Looks good.

@gmcculloug gmcculloug merged commit 2547cca into ManageIQ:master Jan 3, 2019
@gmcculloug gmcculloug added this to the Sprint 102 Ending Jan 7, 2019 milestone Jan 3, 2019
simaishi pushed a commit that referenced this pull request Jan 7, 2019
@gmcculloug @simaishi
Merge pull request #18325 from d-m-u/fixing_retirement_requester_reset
bbf21fc 
Don't reset retirement_requester after end of retirement

(cherry picked from commit 2547cca)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1638502
@simaishi
Copy link
Contributor

simaishi commented Jan 7, 2019

Hammer backport details:

$ git log -1
commit bbf21fc9ccaf0ed3d4d9609b7703ee4b662989ad
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Thu Jan 3 13:52:37 2019 -0500

    Merge pull request #18325 from d-m-u/fixing_retirement_requester_reset
    
    Don't reset retirement_requester after end of retirement
    
    (cherry picked from commit 2547ccaa7c9d0284ae1127585548f3a060f88ccb)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1638502

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 14, 2019

Can we get this backported to g for https://bugzilla.redhat.com/show_bug.cgi?id=1641812, please?
@miq-bot add_label gaprindashvili/yes

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 18, 2019

🦃 🐌 🐍 🗡

@simaishi
Copy link
Contributor

@d-m-u Tried backporting to gaprindashvili branch, but entire file is show up as conflicting for spec/models/orchestration_stack/retirement_management_spec.rb Can you please create a separate PR?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 21, 2019

@simaishi Yes ma'am, just a minute.

@d-m-u d-m-u mentioned this pull request Jan 21, 2019
Merged
@simaishi
Copy link
Contributor

Backported to Gaprindashvili via #18376

@d-m-u d-m-u deleted the fixing_retirement_requester_reset branch February 1, 2019 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gmcculloug gmcculloug gmcculloug approved these changes

@tinaafitz tinaafitz tinaafitz approved these changes

@mkanoor mkanoor Awaiting requested review from mkanoor

Assignees

@gmcculloug gmcculloug

Labels
blocker bug gaprindashvili/backported hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@d-m-u @tinaafitz @simaishi @gmcculloug @miq-bot